-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SGX] Use Fortanix EDP #2885
[SGX] Use Fortanix EDP #2885
Conversation
This seems to be a significant change, perhaps it would be better to open a RFC to elaborate the reasons behind the change. Specifically:
I am not expert but my observation is that our current way of SGX is "closer to the metal", which brings a bit more boilerplate code. This may or may not be a bad thing depending on how stable the code is and how can we evolve. |
ping @nhynes see if you would like to push it through |
Yes, absolutely! I'll give it a go this weekend. |
fe30058
to
cd95366
Compare
Whats the status of this branch? Any chance it's being merged soon? |
Sure! I didn't know if anyone but me was keen on using it, so I've been keeping it WIP as the EDP and Rust evolve. I'll start tidying it up now for a merge hopefully this week. Mind if I ask what you're working on? |
Thanks for updating the branch. Are you able to build this with the most recent rust nightly? I'm still running into this strange compile error. https://discuss.tvm.ai/t/linker-errors-when-compiling-rust-bindings/3712 |
For some reason the frontend was being compiled as a dylib. This PR fixes that. Although, you shouldn't need the frontend if you're just trying to use the Rust runtime. |
gentle ping :) |
this was ready two weeks ago but nobody reviewed it 🤷♂ |
I still get the build error mentioned above if I compile on the latest rust nightly even if I use the default toolchain. |
Latest Rust nightly broke a couple of targets (including SGX), so you have to use 2019-10-11 instead until fixed. See rust-lang/rust#65335 |
Thanks! I think my issue was related to the tvm_runtime being linked to CUDA. When I disable CUDA in the C library the rust bindings from this branch compile fine. |
Could we add a short readme for the rust bindings? In particular which configurations are compatible with the bindings. It seems like CUDA does not work right now but you need to enable LLVM and have the python bindings installed. Also the tests currently fail for me. I get this error
|
sorry that that i lost track of this thread, @nhynes can you rebase and let us merge it in :)? |
@kaimast if you look at how tests are run, you should use limit to |
@@ -214,7 +207,7 @@ impl<'a, 'm> Builder<'a, 'm> { | |||
let (mut values, mut type_codes): (Vec<ffi::TVMValue>, Vec<ffi::TVMTypeCode>) = | |||
self.arg_buf.iter().map(|arg| arg.to_tvm_value()).unzip(); | |||
|
|||
let mut ret_val = unsafe { std::mem::uninitialized::<TVMValue>() }; | |||
let mut ret_val = unsafe { MaybeUninit::uninit().assume_init() }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! you fixed everything :) I was about to send a PR just for that!
@nhynes perhaps we should update to rustc >= 1.40.0. |
ping ping :) |
ping ping @nhynes , let us get this in the new decade :) |
ping @nhynes |
I just thought about this yesterday. I'm not sure I have time to contribute much but development on Rust can't continue without this getting merged as it's such a large PR. |
superseded by #4976 |
Depends on #2616
TODO:
This currently breaks direct use of python, but greatly simplifies the use of TVM in SGX. Future improvements include:
Notes:
Thanks to @paddyhoran for adding docs fixes in #4114